feat: deprecate AggregateUDF: :is_nullable #18934
feat: deprecate AggregateUDF: :is_nullable #18934codetyri0n wants to merge 2 commits intoapache:mainfrom
Conversation
Jefffrey
left a comment
There was a problem hiding this comment.
Surprisingly fewer changes than I expected
7f99f29 to
810cae5
Compare
martin-g
left a comment
There was a problem hiding this comment.
- https://github.com/codetyri0n/datafusion/blob/810cae5c0471a4050e2a9b4ac8580eaf5f039906/datafusion/ffi/src/udaf/mod.rs#L347 is not annotated with
#[expect(deprecated)] - https://github.com/codetyri0n/datafusion/blob/810cae5c0471a4050e2a9b4ac8580eaf5f039906/datafusion/functions-aggregate/src/count.rs#L299 - should something be done for overrides like this one ?
| @@ -209,6 +209,7 @@ impl AggregateUDF { | |||
| } | |||
|
|
|||
| pub fn is_nullable(&self) -> bool { | |||
There was a problem hiding this comment.
Should this method be deprecated too ?
thanks for catching the mod.rs miss @martin-g , i had left the count.rs file untouched as i dont see the signature or the implementation getting altered much in the near future |
810cae5 to
2aeedc8
Compare
|
can we merge this? |
|
Sorry I haven't gotten around to re-reviewing this after the latest changes |
Jefffrey
left a comment
There was a problem hiding this comment.
I think the changes look good in achieving what we want for the issue.
Some notes:
- We should look at UDAFs that implement
is_nullableand ensure they implementreturn_fieldin line with the deprecation; I think this only occurs for Count- Unfortunately it seems Rust can't flag this for us (implementations of deprecated methods)
- I think this PR doesn't introduce behaviour changes so long as any downstream UDAFs that implement
is_nullableeither don't implementreturn_field(in which case the default behaviour still usesis_nullable), or if they do implementreturn_fieldhopefully it is consistent withis_nullableotherwise behaviour may change- That latter case seems like a bug in the downstream anyway, though not sure how to communicate this 🤔
One more thing is I wonder how we should handle this for FFI; for example we have is_nullable as part of the FFI API I believe:
datafusion/datafusion/ffi/src/udaf/mod.rs
Lines 80 to 81 in a1bb74b
Maybe @timsaucer can weigh in regarding FFI?
Apart from that, it would be good if could get some other opinions on if we want to proceed with this deprecation; I know for the UDF is_nullable deprecation there was some concern, see discussions on #14094 and #17074. cc @alamb if you have time
- Maybe we should include a note in the upgrade guide to make this more visible since Rust can't flag the implementation of a deprecated trait method 🤔
|
Two things:
|
|
Thank you for your contribution. Unfortunately, this pull request is stale because it has been open 60 days with no activity. Please remove the stale label or comment or this will be closed in 7 days. |
Which issue does this PR close?
AggregateUDFImpl::is_nullablein favour ofreturn_field#18882Rationale for this change
Are these changes tested?
Are there any user-facing changes?